Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Description of How to add an new Element #2

Closed
wants to merge 1 commit into from
Closed

Description of How to add an new Element #2

wants to merge 1 commit into from

Conversation

damtsnkff
Copy link
Member

I added some description of how to add new components/elements to the current example app.
Overall, do you think the example app approach is good ?
I tried to think about something that would give us some modularity.

@JakeLin
Copy link
Member

JakeLin commented Jul 12, 2016

@damtsnkff thanks for your PR. I saw your preview commit too dd87168 Very nice, 👍

In main project IBAnimatable, there are two targets. IBAnimatable target is a framework tareget. and IBAnimatableApp is an app target, which is a demo app.

I think having an example app is good for the project sepically for developing the new features. We can valify the feature by testing the example app.

Also we don't distribute the example app when we create the framework (using CocoaPods, Carthage or Swift Package Manager) for the users. For IBAnimatable-Material project, we will creat a framework target and move all Animatable<MaterialElements> classes into this target and create a Pod for it. Also create an app target to contain all demo features. MainTableView and all storyboards will be in the app target only. App target will use IBAnimatable-Material target as a framework. Same as what we are doing in `IBAnimatable.

Later on, the user can use CocoaPods, Carthage or Swift Package Manager to use IBAnimatable-Material. IBAnimatable-Material pod will depend on IBAnimatable and Material pods.

Did I answer your question? Please let me know if you have any questions.

@damtsnkff
Copy link
Member Author

Using the same architecture as IBAnimatable makes sense, avoid the confusion, and will be easier to understand. Can I do the modification and then commit on the same PR or should I modify and create another PR ?

@damtsnkff
Copy link
Member Author

To sum up the modifications to make to this PR :

  • Create a Framework target with Animatable<MaterialElements> classes
  • Create an App target, that use the new IBAnimatable-Material Framework
  • Organize the workspace folders in consequence

Is this all for this PR?

@damtsnkff damtsnkff closed this Jul 12, 2016
@damtsnkff damtsnkff deleted the development branch July 12, 2016 16:27
@JakeLin
Copy link
Member

JakeLin commented Jul 12, 2016

Related to #3

@JakeLin
Copy link
Member

JakeLin commented Jul 12, 2016

Hi Damien,

it's up to you to merge current one and create another PR for the frameworks. Or you close it and create a new PR only. We don't have restricted rule. Please let me know if you have any questions.

Jake

Sent from my iPhone

On 13 Jul 2016, at 1:12 AM, Damien Tsnkff [email protected] wrote:

Using the same architecture as IBAnimatable makes sense, avoid the confusion, and will be easier to understand. Can I do the modification and then commit on the same PR or should I modify and create another PR ?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@damtsnkff
Copy link
Member Author

@JakeLin If I merge this one, isn't that going to have conflict with #4 ?
Next PR will be the Framework structure #3 (then another one for the dependency managers #6 ).

@JakeLin
Copy link
Member

JakeLin commented Jul 13, 2016

I won't be a conflict because we can rebase the next one. If there is a conflict, we can do it together to resolve it. Git is very good for resolving conflicts.

Sent from my iPhone

On 13 Jul 2016, at 9:36 AM, Damien Tsnkff [email protected] wrote:

@JakeLin If I merge this one, isn't that going to have conflict with #4 ?
Next PR will be the Framework structure #3 (then another one for the dependency managers #6 ).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants